-
Notifications
You must be signed in to change notification settings - Fork 1.4k
nRF Desktop: application module update after the IronSide SE migration #25461
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
nRF Desktop: application module update after the IronSide SE migration #25461
Conversation
CI InformationTo view the history of this post, click the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 137e4d3ca3a7b8e1e1054fdb71feb258d13a6c52 more detailssdk-nrf:
Github labels
List of changed files detected by CI (3)Outputs:ToolchainVersion: Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
914a872 to
357eb20
Compare
applications/nrf_desktop/doc/dfu.rst
Outdated
| The DFU module is compatible with the memory layout defined using the Partition Manager. | ||
| This partitioning method is used by default for most board targets that are based on the nRF52, nRF53, and nRF54 Series SoCs. | ||
|
|
||
| Additionally, the DFU module is compatible with the memory layout defined using the Device Tree Source (DTS) when it is used with the MCUboot bootloader. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Additionally, the DFU module is compatible with the memory layout defined using the Device Tree Source (DTS) when it is used with the MCUboot bootloader. | |
| Additionally, the DFU module is compatible with the memory layout defined using the Devicetree Source (DTS) when you use it with the MCUboot bootloader. |
Isn't it Devicetree Specification?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://docs.zephyrproject.org/latest/build/dts/intro-syntax-structure.html
As the name indicates, a devicetree is a tree. The human-readable text format for this tree is called DTS (for devicetree source), and is defined in the Devicetree specification.
| @@ -69,6 +69,8 @@ Make sure that the DFU lock utility is enabled if your nRF Desktop application c | |||
| You cannot use this module with the :ref:`caf_ble_smp`. | |||
| In other words, you cannot simultaneously enable the :ref:`CONFIG_DESKTOP_DFU_MCUMGR_ENABLE <config_desktop_app_options>` option and the :kconfig:option:`CONFIG_CAF_BLE_SMP` Kconfig option. | |||
|
|
|||
| Currently, this module supports only one bootloader backend: the MCUboot bootloader backend. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Currently, this module supports only one bootloader backend: the MCUboot bootloader backend. | |
| Currently, this module supports only one bootloader backend, the MCUboot bootloader backend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider combining this sentence with the note below to highlight it (this is the same information):
.. note::
Currently, this module supports only one bootloader backend, the MCUboot bootloader backend.
B0 bootloader is not integrated with MCUmgr.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sentence is an introduction to the subsequent section about the MCUboot backend. I would then duplicate this information.
| @@ -313,6 +316,9 @@ nRF Desktop | |||
| * The documentation of the :ref:`nrf_desktop_hid_state` and default HID report providers to simplify getting started with updating HID input reports used by the application or introducing support for a new HID input report. | |||
| * The default value of the :kconfig:option:`CONFIG_SOC_FLASH_NRF_RADIO_SYNC_MPSL_NORMAL_PRIORITY_TIMEOUT_US` Kconfig option to ``0``. | |||
| This is done to start using high MPSL timeslot priority quicker and speed up non-volatile memory operations. | |||
| * The :ref:`nrf_desktop_dfu` and the :ref:`nrf_desktop_dfu_mcumgr` to handle the ``nrf54h20dk/nrf54h20/cpuapp`` board target with the IronSide SE architecture and the MCUboot bootloader. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should the doc pages be updated with the IronSide SE details?
We have document about IronSide SE here : https://docs.nordicsemi.com/bundle/ncs-latest/page/nrf/app_dev/device_guides/nrf54h/ug_nrf54h20_ironside.html
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so. This is the documentation of the IronSide component, which we do not change.
I think we should just document the nRF Desktop integration with IronSide (or in other words: how we use IronSide in our application).
| @@ -313,6 +316,9 @@ nRF Desktop | |||
| * The documentation of the :ref:`nrf_desktop_hid_state` and default HID report providers to simplify getting started with updating HID input reports used by the application or introducing support for a new HID input report. | |||
| * The default value of the :kconfig:option:`CONFIG_SOC_FLASH_NRF_RADIO_SYNC_MPSL_NORMAL_PRIORITY_TIMEOUT_US` Kconfig option to ``0``. | |||
| This is done to start using high MPSL timeslot priority quicker and speed up non-volatile memory operations. | |||
| * The :ref:`nrf_desktop_dfu` and the :ref:`nrf_desktop_dfu_mcumgr` to handle the ``nrf54h20dk/nrf54h20/cpuapp`` board target with the IronSide SE architecture and the MCUboot bootloader. | |||
|
|
|||
| * Removed the SUIT support from the :ref:`nrf_desktop_dfu` and the :ref:`nrf_desktop_dfu_mcumgr`. | |||
|
|
|||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have mention about suit in the Board configuration docs here : https://docs.nordicsemi.com/bundle/ncs-latest/page/nrf/applications/nrf_desktop/board_configuration.html#nrf_desktop_board_configuration_files. Could remove those line as well? Also, the note about IronSide SE migration at the end of the page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will be done in a follow-up PR. This PR focuses only on the application modules from the nRF Desktop app.
| @@ -313,6 +316,9 @@ nRF Desktop | |||
| * The documentation of the :ref:`nrf_desktop_hid_state` and default HID report providers to simplify getting started with updating HID input reports used by the application or introducing support for a new HID input report. | |||
| * The default value of the :kconfig:option:`CONFIG_SOC_FLASH_NRF_RADIO_SYNC_MPSL_NORMAL_PRIORITY_TIMEOUT_US` Kconfig option to ``0``. | |||
| This is done to start using high MPSL timeslot priority quicker and speed up non-volatile memory operations. | |||
| * The :ref:`nrf_desktop_dfu` and the :ref:`nrf_desktop_dfu_mcumgr` to handle the ``nrf54h20dk/nrf54h20/cpuapp`` board target with the IronSide SE architecture and the MCUboot bootloader. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a plan to update the migration guide as well. Will that be done as part of this PR or a separate PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this PR is one of many for the related JIRA ticket. I plan to contribute the nRF Desktop migration guide at the very end
| @@ -69,6 +69,8 @@ Make sure that the DFU lock utility is enabled if your nRF Desktop application c | |||
| You cannot use this module with the :ref:`caf_ble_smp`. | |||
| In other words, you cannot simultaneously enable the :ref:`CONFIG_DESKTOP_DFU_MCUMGR_ENABLE <config_desktop_app_options>` option and the :kconfig:option:`CONFIG_CAF_BLE_SMP` Kconfig option. | |||
|
|
|||
| Currently, this module supports only one bootloader backend: the MCUboot bootloader backend. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider combining this sentence with the note below to highlight it (this is the same information):
.. note::
Currently, this module supports only one bootloader backend, the MCUboot bootloader backend.
B0 bootloader is not integrated with MCUmgr.
| @@ -313,6 +313,9 @@ nRF Desktop | |||
| * The documentation of the :ref:`nrf_desktop_hid_state` and default HID report providers to simplify getting started with updating HID input reports used by the application or introducing support for a new HID input report. | |||
| * The default value of the :kconfig:option:`CONFIG_SOC_FLASH_NRF_RADIO_SYNC_MPSL_NORMAL_PRIORITY_TIMEOUT_US` Kconfig option to ``0``. | |||
| This is done to start using high MPSL timeslot priority quicker and speed up non-volatile memory operations. | |||
| * The :ref:`nrf_desktop_dfu_mcumgr` to handle the ``nrf54h20dk/nrf54h20/cpuapp`` board target with the IronSide SE architecture and the MCUboot bootloader. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could add one note informing that we added support for the nrf54h20dk/nrf54h20/cpuapp board target with the IronSide SE architecture and the MCUboot bootloader (and SUIT support was removed)? Then we might briefly describe supported features (maybe even as a bullet point sublist)?
We might also add it at the top (to highlight it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This note will be added together with the update of the board configuration from the nRF Desktop documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, I think we could skip adding release notes for now (add add one general note later on). I think it might not be worth to go into details here
| Partitioning methods | ||
| ==================== | ||
|
|
||
| The DFU module is compatible with the memory layout defined using the Partition Manager. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest to provide a bit more context why partitioning is relevant for this module. I would also suggest to remove some duplication. Maybe we could use something similar to:
The DFU module stores the update image received over the configuration channel to a dedicated memory partition (image slot that is currently not in use).
The module is compatible with memory layout defined in either Partition Manager or DTS.
Make sure to properly configure the memory layout in configuration.
For more details about the memory layout, see the :ref:`nrf_desktop_memory_layout` documentation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or maybe we could even skip this section as the information is covered in line 42:
.. important::
The received update image chunks are stored on the dedicated non-volatile memory partition when the current version of the device firmware is running.
For this reason, make sure that you use configuration with a dedicated update image partition.
For more information on configuring the memory layout in the application, see the :ref:`nrf_desktop_memory_layout` documentation.
You could just add information that The module is compatible with memory layout defined in either Partition Manager or DTS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this section would probably be extended if we proceed with the RAM load mode of MCUboot in the context of LM20. Due to this, I would desist from removing this section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add an introductory sentence, as I think it is a good improvement
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this section would probably be extended if we proceed with the RAM load mode of MCUboot in the context of LM20. Due to this, I would desist from removing this section.
That's another reason why we should avoid duplicating information. Maybe we could remove the "generic information" from this doc page and update the nrf_desktop_memory_layout doc page (we need to do it anyway).
357eb20 to
75d4b5b
Compare
|
Force push to address the comments |
Updated the DFU MCUmgr application module that is part of the nRF Desktop application to align with the IronSide SE architecture for the nRF54H20 SoC. Ref: NCSDK-35488 Signed-off-by: Kamil Piszczek <[email protected]>
Updated the DFU application module that is part of the nRF Desktop application to align with the IronSide SE architecture for the nRF54H20 SoC. Ref: NCSDK-35488 Signed-off-by: Kamil Piszczek <[email protected]>
75d4b5b to
137e4d3
Compare
|
Force push to rebase and resolve conflicts. |
| You cannot use this module with the :ref:`caf_ble_smp`. | ||
| In other words, you cannot simultaneously enable the :ref:`CONFIG_DESKTOP_DFU_MCUMGR_ENABLE <config_desktop_app_options>` option and the :kconfig:option:`CONFIG_CAF_BLE_SMP` Kconfig option. | ||
|
|
||
| Currently, this module supports only one bootloader backend, the MCUboot bootloader backend. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would skip this sentence as it's already part of the note below
| @@ -313,6 +313,9 @@ nRF Desktop | |||
| * The documentation of the :ref:`nrf_desktop_hid_state` and default HID report providers to simplify getting started with updating HID input reports used by the application or introducing support for a new HID input report. | |||
| * The default value of the :kconfig:option:`CONFIG_SOC_FLASH_NRF_RADIO_SYNC_MPSL_NORMAL_PRIORITY_TIMEOUT_US` Kconfig option to ``0``. | |||
| This is done to start using high MPSL timeslot priority quicker and speed up non-volatile memory operations. | |||
| * The :ref:`nrf_desktop_dfu_mcumgr` to handle the ``nrf54h20dk/nrf54h20/cpuapp`` board target with the IronSide SE architecture and the MCUboot bootloader. | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then, I think we could skip adding release notes for now (add add one general note later on). I think it might not be worth to go into details here
| Partitioning methods | ||
| ==================== | ||
|
|
||
| The DFU module is compatible with the memory layout defined using the Partition Manager. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this section would probably be extended if we proceed with the RAM load mode of MCUboot in the context of LM20. Due to this, I would desist from removing this section.
That's another reason why we should avoid duplicating information. Maybe we could remove the "generic information" from this doc page and update the nrf_desktop_memory_layout doc page (we need to do it anyway).
No description provided.